airbyte/_util/api_util.py (1)

994-1003: Consider simplifying the boolean expression, wdyt?

Using any({...}) with a set is a bit unconventional. A simple or expression would be more idiomatic and avoids the set construction overhead:

 def _is_safe_name_to_delete(name: str) -> bool:
     """Check if a name is safe to delete.
 
     Requires the name to contain either "delete-me" or "deleteme" (case insensitive).
     """
     name_lower = name.lower()
-    return any({
-        "delete-me" in name_lower,
-        "deleteme" in name_lower,
-    })
+    return "delete-me" in name_lower or "deleteme" in name_lower

airbyte/mcp/cloud_ops.py (3)

907-911: Comment mentions outdated naming pattern - mind updating it?

The inline comment references "delete:" which is not a valid safe-mode pattern anymore. Only "delete-me" or "deleteme" are accepted, wdyt?

     # Safe mode is hard-coded to True for extra protection when running in LLM agents
     workspace.permanently_delete_source(
         source=source_id,
-        safe_mode=True,  # Requires name-based delete disposition ("delete:" or "delete-me")
+        safe_mode=True,  # Requires name to contain "delete-me" or "deleteme"
     )

---

966-970: Same stale comment here too!

     # Safe mode is hard-coded to True for extra protection when running in LLM agents
     workspace.permanently_delete_destination(
         destination=destination_id,
-        safe_mode=True,  # Requires name-based delete disposition ("delete:" or "delete-me")
+        safe_mode=True,  # Requires name to contain "delete-me" or "deleteme"
     )

---

1044-1050: And here as well - third occurrence of the stale comment.

     # Safe mode is hard-coded to True for extra protection when running in LLM agents
     workspace.permanently_delete_connection(
-        safe_mode=True,  # Requires name-based delete disposition ("delete:" or "delete-me")
+        safe_mode=True,  # Requires name to contain "delete-me" or "deleteme"
         connection=connection_id,
         cascade_delete_source=cascade_delete_source,
         cascade_delete_destination=cascade_delete_destination,
     )

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: aaronsteers
Repo: airbytehq/PyAirbyte PR: 411
File: airbyte/cli.py:111-160
Timestamp: 2024-10-06T23:44:31.534Z
Learning: In PyAirbyte, error messages in functions like `_resolve_source_job` in `airbyte/cli.py` are designed to decouple the message text from dynamic values, following a structlog-inspired design. Dynamic values are provided via parameters like `input_value`. This approach helps avoid including PII in the message strings, which may be used in telemetry.

airbyte/mcp/cloud_ops.py (1)

1040-1091: Same workspace_id omission as source deletion.

This function follows the same pattern as permanently_delete_cloud_source and also omits the workspace_id parameter. If you decide to add it to the source function, consider adding it here too for consistency.

airbyte/mcp/cloud_ops.py (2)

981-1032: Missing workspace_id parameter - is this intentional?

I noticed this function doesn't accept a workspace_id parameter, unlike permanently_delete_custom_source_definition (line 924-930) and other similar functions like rename_cloud_source. This means it always uses the default workspace from environment variables.

Was this a deliberate simplification, or should it include the optional workspace_id parameter for consistency with the rest of the API, wdyt?

 def permanently_delete_cloud_source(
     source_id: Annotated[
         str,
         Field(description="The ID of the deployed source to delete."),
     ],
     name: Annotated[
         str,
         Field(description="The expected name of the source (for verification)."),
     ],
+    *,
+    workspace_id: Annotated[
+        str | None,
+        Field(
+            description="Workspace ID. Defaults to AIRBYTE_CLOUD_WORKSPACE_ID env var.",
+            default=None,
+        ),
+    ] = None,
 ) -> str:

---

1099-1171: Cascade delete options look good, same workspace_id note applies.

The cascade delete options (cascade_delete_source, cascade_delete_destination) are well-structured with clear Field descriptions. The safe_mode=True is correctly passed to the underlying call.

Same note about missing workspace_id parameter applies here. The consistency across all three new delete functions suggests this might be intentional though?

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: aaronsteers
Repo: airbytehq/PyAirbyte PR: 411
File: airbyte/cli.py:111-160
Timestamp: 2024-10-06T23:44:31.534Z
Learning: In PyAirbyte, error messages in functions like `_resolve_source_job` in `airbyte/cli.py` are designed to decouple the message text from dynamic values, following a structlog-inspired design. Dynamic values are provided via parameters like `input_value`. This approach helps avoid including PII in the message strings, which may be used in telemetry.

airbyte/_util/api_util.py (3)

747-757: Consider tightening the type check on raw_configuration before splatting

raw_configuration is now optional, which is great, but we still assume it’s a mapping when we do **raw_configuration. Would it be worth guarding with an isinstance to avoid a surprising TypeError if the API ever returns a non-dict configuration (e.g., None or a list) for a known destination type, wdyt?

-        raw_configuration: dict[str, Any] | None = raw_response.get("configuration")
+        raw_configuration: dict[str, Any] | None = raw_response.get("configuration")
@@
-        if destination_type in destination_mapping and raw_configuration is not None:
-            response.destination_response.configuration = destination_mapping[
-                destination_type  # pyrefly: ignore[index-error]
-            ](**raw_configuration)
+        if (
+            destination_type in destination_mapping
+            and isinstance(raw_configuration, dict)
+        ):
+            response.destination_response.configuration = destination_mapping[
+                destination_type  # pyrefly: ignore[index-error]
+            ](**raw_configuration)

---

994-1005: Simplify _is_safe_name_to_delete for readability

The helper behaves correctly, but the any({ ... }) with a set literal is a bit unusual for a simple OR check. Would simplifying this to a direct boolean expression help future readers scan it faster, wdyt?

 def _is_safe_name_to_delete(name: str) -> bool:
@@
-    name_lower = name.lower()
-    return any(
-        {
-            "delete-me" in name_lower,
-            "deleteme" in name_lower,
-        }
-    )
+    name_lower = name.lower()
+    return "delete-me" in name_lower or "deleteme" in name_lower

---

1470-1495: Minor readability tweak and potential DRY opportunity in custom YAML delete

The safe_mode behavior for custom YAML source definitions matches the other delete paths, which is great. Two tiny thoughts:

1. Since you already assign connector_name = definition_info.name, would calling _is_safe_name_to_delete on connector_name read a bit cleaner?
2. Given that the safe_mode blocks and error messages are now very similar across sources, destinations, connections, and custom definitions, would a small internal helper (e.g., _ensure_safe_delete(resource_type, name, id, safe_mode)) help reduce duplication and keep future message changes in one place, wdyt?

-        connector_name = definition_info.name
-
-        if not _is_safe_name_to_delete(definition_info.name):
+        connector_name = definition_info.name
+
+        if not _is_safe_name_to_delete(connector_name):
             raise PyAirbyteInputError(
                 message=(
                     f"Cannot delete custom connector definition '{connector_name}' "
                     "with safe_mode enabled. "
                     "To authorize deletion, the connector name must contain 'delete-me' or "
                     "'deleteme' (case insensitive).\n\n"
                     "Please rename the connector to meet this requirement "

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro